fix(index): support adding index widget with initial UI state #4359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Haroenv
added a commit
to algolia/vue-instantsearch
that referenced
this pull request
Mar 18, 2020
in Vue InstantSearch server side rendering we call .addWidgets in hydrate, and otherwise this would cause an extra search. If we do keep this search then uiState which gets modified in the index widget needs to be changed.
Haroenv
commented
Apr 24, 2020
Haroenv
commented
Apr 27, 2020
samouss
reviewed
May 4, 2020
Haroenv
changed the title
|
E2E test failure seems to be a fluke (cc @yannickcr), locally the URL updates as expected in those scenarios |
nunomaduro
suggested changes
May 5, 2020
Co-authored-by: Nuno Maduro <enunomaduro@gmail.com>
eunjae-lee
approved these changes
May 5, 2020
|
The e2e test seems to actually catch a bug; if a refinement is set again to the default value of the widget (usually nothing) the previous value for uiState is kept by the widget. This is expected on first render, but not later. |
|
The problem is related to this line Before this pull request, every change to the helper, will erase the uiState which is "still there", since it was reduced from an empty object. This was causing issues in Vue InstantSearch, since every widget added on server side erased the initialUiState (except the first widget to be mounted). The solution I saw was to base the new state of the existing state. The issue that pops up because of this is that when a widget refines, and later goes back to its default / unrefined state, all widgets have a guard that check "if not refined, return uiState", and thus they keep the previously refined state in the URL & uiState. A change I can imagine is in all widgets themselves removing that condition and returning a uiState with explicitly |
eunjae-lee
suggested changes
May 5, 2020
eunjae-lee
reviewed
May 6, 2020
Haroenv
commented
May 6, 2020
yannickcr
approved these changes
May 11, 2020
src/widgets/index/index.ts
Outdated
| { | ||
| state, | ||
| isPageReset, | ||
| _isFromAddWidget, |
There was a problem hiding this comment.
(1) Instead of saying where it's from, what about saying what it does? (similarly to isPageReset). This name looses sense suddenly if we were to reuse it somewhere else. A suggestion: _shouldInheritUiState.
(2) To make it more testable and easier to think about, I would rather pass the baseUiState object here, instead of a boolean.
There was a problem hiding this comment.
Good suggestion, I've changed it to _uiState, where we pass the local ui state in this case
francoischalifour
approved these changes
May 13, 2020
Haroenv
commented
May 13, 2020
Haroenv
commented
May 13, 2020
Haroenv
changed the title
Haroenv
added a commit
to algolia/vue-instantsearch
that referenced
this pull request
Jun 9, 2020
* feat(ssr): support multi index * WIP: update nuxt example with new API * different option * Revert "different option" This reverts commit f37fa7e. * remove debug leak * hydrate main index & use in force render * don't init in force render, because addWidget does it alreayd * fixes, ref: algolia/instantsearch#4359 * outdated note * initial next version of ssr todo: tests * more realistic instantsearch integration test * update widget mixin tests based on main index change * move stuff slightly * consistency in test * initial server root tests * commit the patch for now * commit patch * fix: wip * have a more real test * update tests * well discussed test cases & small tweaks * use test utility * actualize bundlesize * update examples * clean up example * remove lint error * suppress another lint * more patches for demo * commit paches * make dumb lint error disappear * fix flaky jest tests * test * update deps * ecom example fix * Update src/util/createServerRootMixin.js Co-authored-by: Eunjae Lee <eunjae.lee@algolia.com> * chore(deps): update e2e tests (#784) * chore(deps): update e2e tests * modern node * ci(circle): update node * ci(examples): use working version of core-js * chore(netlify): remove node modules * chore(ssr): avoid JSON cost * chore(ssr): don't resolve unused things * clean todos * fix typo * change comments Co-authored-by: Vaillant Samuel <samuel.vllnt@gmail.com> Co-authored-by: Eunjae Lee <eunjae.lee@algolia.com>
Haroenv
added a commit
to algolia/vue-instantsearch
that referenced
this pull request
Jun 11, 2020
* feat(ssr): support multi index * WIP: update nuxt example with new API * different option * Revert "different option" This reverts commit f37fa7e. * remove debug leak * hydrate main index & use in force render * don't init in force render, because addWidget does it alreayd * fixes, ref: algolia/instantsearch#4359 * outdated note * initial next version of ssr todo: tests * more realistic instantsearch integration test * update widget mixin tests based on main index change * move stuff slightly * consistency in test * initial server root tests * commit the patch for now * commit patch * fix: wip * have a more real test * update tests * well discussed test cases & small tweaks * use test utility * actualize bundlesize * update examples * clean up example * remove lint error * suppress another lint * more patches for demo * commit paches * make dumb lint error disappear * fix flaky jest tests * test * update deps * ecom example fix * Update src/util/createServerRootMixin.js Co-authored-by: Eunjae Lee <eunjae.lee@algolia.com> * chore(deps): update e2e tests (#784) * chore(deps): update e2e tests * modern node * ci(circle): update node * ci(examples): use working version of core-js * chore(netlify): remove node modules * chore(ssr): avoid JSON cost * chore(ssr): don't resolve unused things * clean todos * fix typo * change comments Co-authored-by: Vaillant Samuel <samuel.vllnt@gmail.com> Co-authored-by: Eunjae Lee <eunjae.lee@algolia.com>
Haroenv
added a commit
that referenced
this pull request
Dec 28, 2022
…arch#764) * feat(ssr): support multi index * WIP: update nuxt example with new API * different option * Revert "different option" This reverts commit f37fa7e626be09af6c31fe3cf4bd310967319aef. * remove debug leak * hydrate main index & use in force render * don't init in force render, because addWidget does it alreayd * fixes, ref: #4359 * outdated note * initial next version of ssr todo: tests * more realistic instantsearch integration test * update widget mixin tests based on main index change * move stuff slightly * consistency in test * initial server root tests * commit the patch for now * commit patch * fix: wip * have a more real test * update tests * well discussed test cases & small tweaks * use test utility * actualize bundlesize * update examples * clean up example * remove lint error * suppress another lint * more patches for demo * commit paches * make dumb lint error disappear * fix flaky jest tests * test * update deps * ecom example fix * Update src/util/createServerRootMixin.js Co-authored-by: Eunjae Lee <eunjae.lee@algolia.com> * chore(deps): update e2e tests (algolia/vue-instantsearch#784) * chore(deps): update e2e tests * modern node * ci(circle): update node * ci(examples): use working version of core-js * chore(netlify): remove node modules * chore(ssr): avoid JSON cost * chore(ssr): don't resolve unused things * clean todos * fix typo * change comments Co-authored-by: Vaillant Samuel <samuel.vllnt@gmail.com> Co-authored-by: Eunjae Lee <eunjae.lee@algolia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorry for this larger pull request, if it proves too complicated for a single PR, we can split this up in three smaller PRs
since every added widget calls scheduleSearch if
inithas happened, this used to do a search, even beforestartwas called, and thus with not yet the correct statee.g. initialUiState contains a few widgets, if you mount those one by one, after
initon its index, each widget would cause a change event on the helper, which used to erase non-valid state, since it starts from an empty ui state. This is now fixed by using the previous local ui state as base, but a special provision forisPageResetIt's okay to do
results.hits = ...inside connectHits, since that overrides just the value onSearchResults, however if we mutate within the inner hits, we are actually mutating_rawResults, which isn't what we want to achieve for server side rendering, since_rawResultsgets serialised.Further changes are just minor for tests
How to test this?
Two options: